fix(deploy): prevent code-generation injection from angular.json values - #3739

Thanks for this, and for keeping at the ng deploy hardening. I reproduced what you describe: rendering the generated function with a crafted server outputPath gives a standalone require('child_process').execSync(...) statement in index.js, and a crafted functionsNodeVersion adds its own RUN line to the Dockerfile.

There is one gap I think should be filled before this merges, and a few smaller notes that should not hold it up.

Blocking: two more angular.json values still reach the generated function unguarded

functionName and region come from the same deploy options block as functionsNodeVersion, and both are written straight into the same generated index.js with no validation:

• functionName is written as a bare identifier:
  • exports.${functionName || DEFAULT_FUNCTION_NAME} sits in functions-templates.ts at lines 44 and 62, so this applies to both the default and the CF3v2 template.
  • A value of ssr; require('child_process').execSync('...'); var _x renders as exports.ssr; followed by the injected call, with the template's own = assignment becoming that variable's initializer.
  • The file still parses, so the injected call runs when the function loads.
• region is written inside a quoted string:
  • .region('${options.region || DEFAULT_FUNCTION_REGION}') at line 45 puts it in a single-quoted literal in the default template, so a ' breaks out exactly the way outputPath did.
  • The CF3v2 template passes region through JSON.stringify, so that path is already safe.

Neither has a pattern in schema.json (functionName and region are both plain type: string), so nothing upstream constrains them either.

Since the description says this prevents code-generation injection from angular.json values, either of these would unblock it for me:

• Extend the fix with a check on functionName and one on region, called before the template runs, matching the pattern you already established. This is the outcome I would prefer.
• Narrow the title and description to the two values this covers, and open a short follow-up issue for functionName and region, so the change matches its claim and the remaining exposure stays tracked rather than closed over.

Non-blocking notes

• Consider escaping where the value is written rather than only screening it on the way in. Your validators are a blocklist of dangerous characters, which has to stay ahead of every context the value lands in. Two places that could be structural instead:
  • For a string position, emit ${JSON.stringify(value)} and drop the quotes already in the template, since JSON.stringify supplies its own. Written as .region(${JSON.stringify(...)}) it escapes correctly, whereas leaving the existing quotes in place would yield .region('"us-central1"') and change the value.
  • For the exports.<name> position, an allowlist of valid JavaScript identifiers is easier to reason about than a list of rejects. It would also make a currently silent failure loud: a functionName containing a dash already generates a file that does not parse.
• A leading dash still gets through.
  • The character list does not reject -, and on the Cloud Run path the generated package.json sets start: node <serverOutputPath>/main.js.
  • So a server outputPath beginning with - reaches node as a flag rather than a path.
  • The comment above the check says a legitimate output directory never contains these characters, which reads stronger than what the character class enforces.
• Nothing fails if the checks stop being called.
  • The new specs exercise assertSafeOutputPath and assertSafeNodeVersion directly.
  • What I could not find is a test that fails if the builder stops calling them: removing the calls from deployToFunction and deployToCloudRun still passes the whole suite.
  • A test that drives one of those functions with a hostile outputPath and expects a throw would keep the protection from quietly disappearing later.
• Spec count in the description.
  • Locally npm run test:node reports 78 specs on this branch, not the 150 in the body.
  • The branch is based on an older commit, so a rebase on current main would refresh that number.

If I have misread any of this, point me at it and I will take another look.